_{Comment @coderabbitai help to get the list of available commands.}

🤖 Review · ⚠️ Cancelled · Started 3:21 PM UTC · Ended 3:25 PM UTC
Commit: 87c4a29 · View workflow run →

🤖 Finished Review · ✅ Success · Started 3:26 PM UTC · Completed 3:31 PM UTC
Commit: 87c4a29 · View workflow run →

Review — Approve

PR: #3412 — Include enforcement date in warnings for future effective_on violations
Author: Acepresso · Status: Draft · Files changed: 3 · Diff: +111 / −3

Summary

When the CLI demotes a deny/failure result to a warning because its effective_on date is in the future, the warning message is now enhanced to indicate when enforcement begins (e.g., "The required 'cpe' label is missing. This will become a failure starting on 2025-01-01T00:00:00Z").

The change introduces a helper function formatFutureEnforcementMessage and applies it in both LegacyPostEvaluationFilter.CategorizeResults and UnifiedPostEvaluationFilter.CategorizeResults. The helper is well-guarded: it returns the original message unchanged when metadata is nil, the effective_on key is missing, or the value is non-string or empty. Implicitly, the isResultEffective gate in the calling code ensures only validly-parseable future dates reach the enhancement path.

Findings

All findings are low severity — none are blocking.

1. strings.TrimRight vs strings.TrimSuffix — subtle behavior difference

Severity: low · File: internal/evaluator/filters.go · Line: ~1233

strings.TrimRight(originalMessage, ".") removes all trailing characters in the cutset, not just a single trailing period. If a message ever ends with "..." (e.g., an ellipsis), all three dots are stripped. strings.TrimSuffix(originalMessage, ".") would remove exactly one trailing period, which is the likely intent.

In practice, policy violation messages rarely end with ellipses, so the risk is cosmetic only.

Suggested fix:

return fmt.Sprintf("%s. This will become a failure starting on %s",
    strings.TrimSuffix(originalMessage, "."), effectiveOnStr)

2. Duplicated logic across Legacy and Unified filter implementations

Severity: low · File: internal/evaluator/filters.go · Lines: ~1000–1008, ~1125–1133

The same 7-line block (inner if !isResultEffective → create enhancedResult → append) is copy-pasted into both LegacyPostEvaluationFilter.CategorizeResults and UnifiedPostEvaluationFilter.CategorizeResults. If this logic changes in the future (e.g., message format update, additional guards), both sites must be updated in lockstep.

Consider extracting a small helper like:

func appendWithEnhancement(result Result, effectiveTime time.Time, warnings *[]Result) {
    if !isResultEffective(result, effectiveTime) {
        result.Message = formatFutureEnforcementMessage(result)
    }
    *warnings = append(*warnings, result)
}

This would eliminate the duplication and make future changes single-site.

3. Edge case: severity: warning + future effective_on produces misleading message

Severity: low · File: internal/evaluator/filters.go · Lines: ~1000, ~1125

When a deny result carries both severity: warning (demotes it to a warning by annotation) and a future effective_on date, the outer condition getSeverity(result) == severityWarning || !isResultEffective(result, effectiveTime) is satisfied by both disjuncts. The inner if !isResultEffective(...) then fires, appending "This will become a failure starting on ." However, after that date passes, the severity: warning annotation still keeps the result as a warning — it never actually becomes a failure. The message is technically misleading.

This requires contradictory rule configuration (severity: warning plus a future enforcement date is unusual), so real-world impact is negligible. No fix needed now, but worth a comment in the code if the team agrees.

Verification

• ✅ The formatFutureEnforcementMessage helper is thoroughly unit-tested (6 cases: valid date, missing metadata, non-string value, empty string, nil metadata, trailing period)
• ✅ The existing TestConftestEvaluatorEvaluateSeverity integration test correctly updated expected output
• ✅ isResultEffective returns true for unparseable dates, preventing the enhancement branch from executing with invalid date strings — good implicit validation
• ✅ No cross-repo contract changes — Result.Message is user-facing text, not an API boundary
• ✅ No security concerns
• ⚠ No test coverage for the UnifiedPostEvaluationFilter.CategorizeResults path (the duplicated logic), though the implementation is identical